-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MWPW-157555: Remove useless logic and putting import commerce.js top. #2906
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2906 +/- ##
==========================================
+ Coverage 96.24% 96.30% +0.05%
==========================================
Files 236 242 +6
Lines 54233 54979 +746
==========================================
+ Hits 52196 52946 +750
+ Misses 2037 2033 -4 ☔ View full report in Codecov by Sentry. |
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me :) may be some more talkative commit message would be great :)
libs/blocks/merch/merch.js
Outdated
@@ -2,6 +2,7 @@ import { | |||
createTag, getConfig, loadArea, loadScript, loadStyle, localizeLink, | |||
} from '../../utils/utils.js'; | |||
import { replaceKey } from '../../features/placeholders.js'; | |||
import '../../deps/mas/commerce.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? This is also being imported in L427
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we want to start to load this ASAP when merch.js get loaded.
This is what task's description says:
"Remove the related logic and move import of commerce.js to the top."
@seanchoi-dev the following test should be removed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will block to prevent accidental merge until the test is fixed and comments are resolved.
I removed it. |
This reverts commit eae08d2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanchoi-dev can you provide valid before/after URLs for PSI checks?
|
* stage: MWPW-158886: update the way we send arbitrary fields to caas (adobecom#2977) MWPW-159028: MAS more technical documents + refactored aem related code (adobecom#2952) MWPW-158776 Fix video CLS (adobecom#2915) MWPW-159197 Invoke onReady of standalone gnav when it is rendered adobecom#2962 (adobecom#2963) MWPW-159157: Update modal to account for new image size (adobecom#2975) Reverts MWPW-157555: Causing prices and CTAs to fail in Safari (adobecom#2987) [MWPW-159287 : NALA] Enhance Nala Run Command to Support Feature Branches with with an owner other than 'adobecom' (adobecom#2978) [MWPW-158927] Countdown Timer feedbacks incorporated (adobecom#2955) [MWPW-159265] Callout text in RTL not properly aligned (adobecom#2974) Add support for Jarvis through Unav and make appID of notifications configurable (adobecom#2969) MWPW-159236 Fix duplicate percent in donut chart tooltip (adobecom#2968) MWPW-144360 Prototype pollution tag selector (adobecom#2967) [MWPW-153616] Update 404 links in region selector (adobecom#2956) MWPW-157555: Remove useless logic and putting import commerce.js top. (adobecom#2906) # Conflicts: # libs/deps/mas/mas.js # libs/deps/mas/merch-card.js # libs/deps/mas/plans-modal.js # libs/features/mas/web-components/src/global.css.js
…adobecom#2906) * remove useless logic and putting import '../../deps/mas/commerce.js'; top. * removing unsed test that failing * putting commerceLib on top * Revert "putting commerceLib on top" This reverts commit eae08d2. * remove code that is not needed anymore
Resolves: MWPW-157555
Test URLs:
Consumer tests: